dev/mail#20 : Preview screen doesn't open until recipients list is built on mail compose screen #12509

monishdeb · 2018-07-18T18:38:45Z

Overview

To replicate the issue:

Create a new mailing and fill out everything but not the Recipients field.
Now select a recipients group that is either very large or is based on a complex smart group. after selecting it -- click preview HTML/Text.

Before

The preview window will not open until the recipient count query has completed. This can also be replicated by setting up a mailing with a large/complex group, saving it, then continuing it and immediately clicking preview. the window will not open until the query completes.

After

Preview action doesn't wait for recipient built to complete and opens the preview window instantly.

Technical Details

As per the change it directly calls API and doesn't rely on Promise queue to deliver the result in sequence.

civibot · 2018-07-18T18:38:47Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

monishdeb · 2018-07-18T18:38:58Z

ping @lcdservices @colemanw

eileenmcnaughton · 2018-07-18T23:18:24Z

also @davejenx

pradpnayak · 2018-07-19T12:26:20Z

ang/crmMailing/services.js

@@ -286,6 +286,15 @@
        }
      },

+      // @param mailing Object (per APIv3)
+      // @return preview content
+      quickPreview: function quickPreview(mailing) {


why don't we catch only mailing.id rather whole mailing object or array?

monishdeb · 2018-07-20T13:28:42Z

Thanks @pradpnayak for your feedback. I have made that change, please have a look.

lcdservices · 2018-07-20T15:58:22Z

@monishdeb tested and working as expected. The preview modal opens immediately.

eileenmcnaughton · 2018-07-20T21:49:23Z

@seamuslee001 @andrew-cormick-dockery This has been tested but I'd really like you guys to sign off on it too as you got stung by a mailing bug recently

monishdeb · 2018-08-06T11:59:25Z

@lcdservices I have changed my patch, which will now fix the issue where earlier, making any changes on HTML/text content doesn't get reflected in the preview screen, as the content is not saved to DB. On another look, I saw that we don't even need to call Mailing.preview API as we already have a variable $scope.mailing which holds the mailing instance and we use it directly and thus resolve all issues

eileenmcnaughton · 2018-08-06T19:38:56Z

test this please

…t on mail compose screen

lcdservices · 2018-08-13T12:49:07Z

@Monish this looks good. The preview does not depend on the recipients list being built before displaying, and when clicked, will display the mailing content even if autosave is not completed.

colemanw · 2018-08-28T21:11:15Z

@seamuslee001 or @andrew-cormick-dockery I think this is waiting on the green light from one of you.

seamuslee001 · 2018-08-28T21:34:37Z

I've reviewed the code and gave this a test using a pretty big mailing and worked as expected. I can't see any ACL issues here. Happy to see this merged

Flexmailer overrides the `Mailing.preview` API. In civicrm/civicrm-core#12509 (5.6), it became possible to call `Mailing.preview` without an `id` for the mailing. This update Flexmailer `Mailing.preview` to also work without an ID.

Flexmailer overrides the `Mailing.preview` API. In civicrm/civicrm-core#12509 (5.6), it became possible to call `Mailing.preview` without an `id` for the mailing. This update Flexmailer `Mailing.preview` to also work without an ID, and it expands unit-test coverage to ensure correct operation with or without an ID.

…essor/Flexmailer Overview -------- When using `TokenProcessor` to generate a mailing (e.g. as with Flexmailer/Mosaico), the action-tokens (e.g. `{action.optOutUrl}`) are generated via `CRM_Mailing_ActionTokens`. To properly generate them, `CRM_Mailing_ActionTokens` relies on certain information (e.g. mailing/job ID). However, that information is no longer available when performing a "Preview" -- leading to misbehavior in previews. This patch allows Flexmailer to restore parity for previewing those tokens. Note: This is a pre-requisite for the full fix civicrm/org.civicrm.flexmailer#38 Before (Pre-5.6) ---------------- * When a user begins composing a mailing, CiviMail creates a draft mailing with a concrete ID (e.g. `mailing civicrm#123`). * To preview the mailing, the UI calls `Mailing.preview` API with the ID of the mailing. * Flexmailer/Mosaico generates the preview by calling `TokenProcessor` and therefore `CRM_Mailing_ActionTokens`. * `CRM_Mailing_ActionTokens` has strictness checks. These pass because the ID is available. Before (5.6-5.12) ---------------- As a performance enhancement, CiviCRM 5.6 (PR civicrm#12509; [dev/mail#20](https://lab.civicrm.org/dev/mail/issues/20)) revised the signature for `Mailing.preview` API to allow previews *without* having a specific mailing record/job/ID. Consequently: * When a user begins composing a mailing, CiviMail creates a draft mailing with a concrete ID (e.g. `mailing civicrm#123`). * To preview the mailing, the UI calls `Mailing.preview` API ~~with~~ **without** the ID of the mailing. * Flexmailer/Mosaico generates the preview by calling `TokenProcessor` and therefore `CRM_Mailing_ActionTokens`. * `CRM_Mailing_ActionTokens` has strictness checks. These ~~pass~~ **fail** because the ID is ~~available~~ **unavailable**. After ---------------- * When a user begins composing a mailing, CiviMail creates a draft mailing with a concrete ID (e.g. `mailing civicrm#123`). * To preview the mailing, the UI calls `Mailing.preview` API ~~with~~ **without** the ID of the mailing. * Flexmailer/Mosaico generates the preview by calling `TokenProcessor` and therefore `CRM_Mailing_ActionTokens`. * `CRM_Mailing_ActionTokens` has ~~strictness~~ **less strict** checks. These **pass** because the `context[schema]` hints that a mailing ID *will be available* when needed.

…x (dev/mail#20) Flexmailer overrides the `Mailing.preview` API. In civicrm/civicrm-core#12509 (5.6), it became possible to call `Mailing.preview` without an `id` for the mailing. This update Flexmailer `Mailing.preview` to also work without an ID, and it expands unit-test coverage to ensure correct operation with or without an ID.

…essor/Flexmailer Overview -------- When using `TokenProcessor` to generate a mailing (e.g. as with Flexmailer/Mosaico), the action-tokens (e.g. `{action.optOutUrl}`) are generated via `CRM_Mailing_ActionTokens`. To properly generate them, `CRM_Mailing_ActionTokens` relies on certain information (e.g. mailing/job ID). However, that information is no longer available when performing a "Preview" -- leading to misbehavior in previews. This patch allows Flexmailer to restore parity for previewing those tokens. Before (Pre-5.6) ---------------- * When a user begins composing a mailing, CiviMail creates a draft mailing with a concrete ID (e.g. `mailing civicrm#123`). * To preview the mailing, the UI calls `Mailing.preview` API with the ID of the mailing. * Flexmailer/Mosaico generates the preview by calling `TokenProcessor` and therefore `CRM_Mailing_ActionTokens`. * `CRM_Mailing_ActionTokens` has strictness checks. These pass because the ID is available. Before (5.6-5.12) ---------------- As a performance enhancement, CiviCRM 5.6 (PR civicrm#12509; [dev/mail#20](https://lab.civicrm.org/dev/mail/issues/20)) revised the signature for `Mailing.preview` API to allow previews *without* having a specific mailing record/job/ID. Consequently: * When a user begins composing a mailing, CiviMail creates a draft mailing with a concrete ID (e.g. `mailing civicrm#123`). * To preview the mailing, the UI calls `Mailing.preview` API ~~with~~ **without** the ID of the mailing. * Flexmailer/Mosaico generates the preview by calling `TokenProcessor` and therefore `CRM_Mailing_ActionTokens`. * `CRM_Mailing_ActionTokens` has strictness checks. These ~~pass~~ **fail** because the ID is ~~available~~ **unavailable**. After ---------------- * When a user begins composing a mailing, CiviMail creates a draft mailing with a concrete ID (e.g. `mailing civicrm#123`). * To preview the mailing, the UI calls `Mailing.preview` API ~~with~~ **without** the ID of the mailing. * Flexmailer/Mosaico generates the preview by calling `TokenProcessor` and therefore `CRM_Mailing_ActionTokens`. * `CRM_Mailing_ActionTokens` has ~~strictness~~ **less strict** checks. These **pass** because the `context[schema]` hints that a mailing ID *will be available* when needed.

civicrm-builder added the master label Jul 18, 2018

pradpnayak reviewed Jul 19, 2018

View reviewed changes

monishdeb force-pushed the dev-mail-20 branch from c54bcbd to f646e62 Compare July 20, 2018 13:22

JoeMurray changed the title ~~dev/mail#20 : Preview screen don't open until recipients list is built on mail compose screen~~ dev/mail#20 : Preview screen doesn't open until recipients list is built on mail compose screen Jul 20, 2018

monishdeb force-pushed the dev-mail-20 branch from f646e62 to 73e71ae Compare August 6, 2018 11:53

monishdeb force-pushed the dev-mail-20 branch from 73e71ae to 6bb7fcf Compare August 6, 2018 12:04

monishdeb force-pushed the dev-mail-20 branch 2 times, most recently from 349b78f to a76869e Compare August 6, 2018 20:56

dev/mail#20 : Preview screen don't open until recipients list is buil…

24689c7

…t on mail compose screen

monishdeb force-pushed the dev-mail-20 branch from a76869e to ea0358f Compare August 11, 2018 20:57

dev/mail#20 : Preview screen don't open until recipients list is buil…

0075b0c

…t on mail compose screen

monishdeb force-pushed the dev-mail-20 branch from ea0358f to 0075b0c Compare August 13, 2018 08:58

colemanw merged commit bdf8fa2 into civicrm:master Aug 28, 2018

monishdeb deleted the dev-mail-20 branch August 30, 2018 18:55

totten mentioned this pull request Apr 26, 2019

(dev/mail#20) Mailing.preview - Fix regression circa Civi 5.6.x civicrm/org.civicrm.flexmailer#38

Merged

totten mentioned this pull request Apr 30, 2019

CiviMail - Restore support for preview of "mailing"/"action" tokens via TokenProcessor/Flexmailer #14156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/mail#20 : Preview screen doesn't open until recipients list is built on mail compose screen #12509

dev/mail#20 : Preview screen doesn't open until recipients list is built on mail compose screen #12509

monishdeb commented Jul 18, 2018

civibot bot commented Jul 18, 2018

monishdeb commented Jul 18, 2018

eileenmcnaughton commented Jul 18, 2018

pradpnayak Jul 19, 2018

monishdeb commented Jul 20, 2018

lcdservices commented Jul 20, 2018

eileenmcnaughton commented Jul 20, 2018

monishdeb commented Aug 6, 2018

eileenmcnaughton commented Aug 6, 2018

lcdservices commented Aug 13, 2018

colemanw commented Aug 28, 2018

seamuslee001 commented Aug 28, 2018

dev/mail#20 : Preview screen doesn't open until recipients list is built on mail compose screen #12509

dev/mail#20 : Preview screen doesn't open until recipients list is built on mail compose screen #12509

Conversation

monishdeb commented Jul 18, 2018

Overview

Before

After

Technical Details

civibot bot commented Jul 18, 2018

monishdeb commented Jul 18, 2018

eileenmcnaughton commented Jul 18, 2018

pradpnayak Jul 19, 2018

Choose a reason for hiding this comment

monishdeb commented Jul 20, 2018

lcdservices commented Jul 20, 2018

eileenmcnaughton commented Jul 20, 2018

monishdeb commented Aug 6, 2018

eileenmcnaughton commented Aug 6, 2018

lcdservices commented Aug 13, 2018

colemanw commented Aug 28, 2018

seamuslee001 commented Aug 28, 2018